ECOPROJECT-4368 | feat: Add authz for group endpoint#1097
Merged
Conversation
tupyy
requested review from
AvielSegev and
ronenav
and removed request for
a team
📝 WalkthroughWalkthroughAdded explicit 403 responses in the OpenAPI and regenerated client/server artifacts; introduced AccountsServicer and admin-group YAML parsing/Initialize; added AuthzAccountsService enforcing admin checks; wired admin-group initialization and conditional authz wrapping at server startup; updated handlers and tests to surface 403 flows. ChangesOpenAPI / Generated API
Accounts service, parsing, and authz
Handlers & Tests
Server wiring, config, and deployment
Sequence Diagram(s)sequenceDiagram
participant Server
participant Config
participant Parser
participant AccountsSvc
participant Store
Server->>Config: read MIGRATION_PLANNER_ADMIN_GROUP_FILE
alt AdminGroupFile set
Server->>Parser: ParseAdminGroupFile(path)
Parser->>Parser: read YAML, validate -> AdminGroup
Parser-->>Server: AdminGroup
Server->>AccountsSvc: Initialize(ctx, AdminGroup)
AccountsSvc->>Store: Begin tx, delete existing admin groups/members
loop members
AccountsSvc->>Store: create member + group links
end
AccountsSvc->>Store: commit tx
AccountsSvc-->>Server: success
end
sequenceDiagram
participant Client
participant Handler
participant AuthzAccountsService
participant InnerAccountsService
participant Store
Client->>Handler: ListGroups request
Handler->>AuthzAccountsService: ListGroups(ctx)
AuthzAccountsService->>AuthzAccountsService: requireAdmin(ctx)
alt user is admin
AuthzAccountsService->>InnerAccountsService: ListGroups(ctx)
InnerAccountsService->>Store: query groups
Store-->>InnerAccountsService: groups
InnerAccountsService-->>AuthzAccountsService: groups
AuthzAccountsService-->>Handler: groups
Handler-->>Client: 200 OK
else user not admin
AuthzAccountsService-->>Handler: ErrForbidden
Handler-->>Client: 403 Forbidden
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/service/accounts.go (1)
3-13:⚠️ Potential issue | 🟡 MinorNormalize admin group values before validation.
Whitespace-only
name,username, or🛡️ Proposed validation tightening
import ( "context" "errors" "fmt" "os" + "strings" "github.com/google/uuid" "github.com/kubev2v/migration-planner/internal/auth" "github.com/kubev2v/migration-planner/internal/store" "github.com/kubev2v/migration-planner/internal/store/model" @@ if err := yaml.Unmarshal(data, &ag); err != nil { return nil, fmt.Errorf("parsing admin group file: %w", err) } + ag.Name = strings.TrimSpace(ag.Name) if ag.Name == "" { return nil, fmt.Errorf("admin group file: name is required") } if len(ag.Members) == 0 { return nil, fmt.Errorf("admin group file: at least one member is required") } - for i, m := range ag.Members { - if m.Username == "" { + for i := range ag.Members { + ag.Members[i].Username = strings.TrimSpace(ag.Members[i].Username) + ag.Members[i].Email = strings.TrimSpace(ag.Members[i].Email) + if ag.Members[i].Username == "" { return nil, fmt.Errorf("admin group file: member[%d] username is required", i) } - if m.Email == "" { + if ag.Members[i].Email == "" { return nil, fmt.Errorf("admin group file: member[%d] email is required", i) } }As per coding guidelines, “Input validation is mandatory.”
Also applies to: 41-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/accounts.go` around lines 3 - 13, Trim whitespace from admin group fields before performing required-field validation: in internal/service/accounts.go update the validation path that checks admin group "name", "username", and "email" so each value is normalized with strings.TrimSpace(...) prior to the existing non-empty checks (and add the strings import). Apply this normalization immediately after YAML/unmarshal or when constructing the admin group object so whitespace-only values fail validation the same as empty values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/handlers/v1alpha1/accounts_test.go`:
- Line 43: Update the tests in internal/handlers/v1alpha1/accounts_test.go to
exercise handler-level 403 mappings by replacing or augmenting the injected
service used to construct handlers.NewServiceHandler: instead of
service.NewAccountsService(s) inject an authorization-wrapped service
(service.NewAuthzAccountsService(...)) with a non-admin token context, or mock
the accounts service to return service.ErrForbidden for the group/member
methods; add test cases that call the handlers' ListGroups, CreateGroup,
GetGroup, UpdateGroup, DeleteGroup, ListGroupMembers, CreateGroupMember,
UpdateGroupMember, and RemoveGroupMember endpoints and assert they produce HTTP
403 responses when the underlying service returns ErrForbidden or the authz
wrapper denies access.
In `@internal/service/accounts_test.go`:
- Around line 502-556: Add validation for whitespace-only fields in
ParseAdminGroupFile: trim string fields before checking emptiness (e.g., ensure
ag.Name, each member.Username and member.Email are checked as
strings.TrimSpace(value) == "") and return the same error messages used for
empty strings. Also extend the tests in internal/service/accounts_test.go by
adding parallel cases to the existing ones that write whitespace-only values
(e.g., "name: \" \"", "username: \" \"", "email: \" \"", and "members: [ ]"
as appropriate) and assert the same error substrings; use the same test helpers
and filenames as the current tests to keep structure consistent.
- Around line 400-427: Add a new test (e.g., "replaces existing admin groups on
re-initialize when multiple match") that seeds two AdminGroup entries with the
same Name (using service.AdminGroup and service.AdminGroupMember) before calling
svc.Initialize(context.TODO(), ag2); after Initialize call use
svc.ListGroups(context.TODO(), nil) and assert there is a single group with the
new members and that no stale members from either of the original groups remain
(check group count, members length, and that members contain only the new
usernames like "bob" and "carol"). Ensure the test mirrors the structure of the
existing "replaces existing admin group on re-initialize" but creates two
initial groups to trigger removal of all matching stale groups.
In `@internal/service/accounts.go`:
- Around line 140-174: The Initialize method currently deletes the existing
admin group then creates a new group and members with separate store calls
(ListGroups, DeleteGroup, CreateGroup, CreateMember), which can leave startup in
a partial state and bypass member authz side effects; change this to a single
atomic operation in the accounts store (e.g., add a transactional method like
ReplaceAdminGroup or CreateOrReplaceAdminGroup) that performs the delete, create
group, and create-member (using the normal CreateMember/authz path) inside one
DB transaction and rolls back on any error, then call that new transactional
method from Initialize and propagate wrapped errors.
---
Outside diff comments:
In `@internal/service/accounts.go`:
- Around line 3-13: Trim whitespace from admin group fields before performing
required-field validation: in internal/service/accounts.go update the validation
path that checks admin group "name", "username", and "email" so each value is
normalized with strings.TrimSpace(...) prior to the existing non-empty checks
(and add the strings import). Apply this normalization immediately after
YAML/unmarshal or when constructing the admin group object so whitespace-only
values fail validation the same as empty values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fd7894a8-4d18-491e-ab46-d81692ae2541
📒 Files selected for processing (13)
api/v1alpha1/openapi.yamlgo.modinternal/api/server/server.gen.gointernal/api_server/server.gointernal/config/config.gointernal/handlers/v1alpha1/accounts.gointernal/handlers/v1alpha1/accounts_test.gointernal/handlers/v1alpha1/partner_test.gointernal/handlers/v1alpha1/source.gointernal/service/accounts.gointernal/service/accounts_test.gointernal/service/authz_accounts.gointernal/service/partner_test.go
tupyy
force-pushed
the
auhtz-groups
branch
4 times, most recently
from
6bb8322 to
f282531
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
internal/service/accounts.go (1)
155-185:⚠️ Potential issue | 🟠 Major
Initializestill bypasses authz relationship bookkeeping for admin members (and leaves orphans on delete).Two consistency gaps with the regular
CreateMember/RemoveGroupMemberpaths (see lines 278‑284 and 354‑359 in this same file):
- Lines 161‑165: deleting a pre‑existing admin group via
s.store.Accounts().DeleteGroupdoes not issue aWithout(...)WriteRelationshipsfor its previous members, so stalememberReBAC tuples remain under that (now‑deleted) org resource.- Lines 177‑185:
s.store.Accounts().CreateMemberis called directly on the store, skipping the service‑levelCreateMemberwhich also writes theMemberRelationtuple vias.store.Authz().WriteRelationships. Newly bootstrapped admins therefore have no org membership relationship in the authz backend.
requireAdminusesGetIdentity(group Kind == "admin") so login still works, but any ReBAC check keyed onorg/member(user)for admins (e.g., future cross‑resource rules, consistency of the authz store) will be wrong. Since all writes here already run inside the transaction context, both fixes are cheap.🔒 Proposed fix
for _, g := range groups { + // Collect members first so we can drop their authz tuples after deletion. + oldMembers, err := s.store.Accounts().ListMembers(ctx, store.NewMemberQueryFilter().ByGroupID(g.ID)) + if err != nil { + return fmt.Errorf("listing members of admin group %s: %w", g.ID, err) + } if err := s.store.Accounts().DeleteGroup(ctx, g.ID); err != nil { return fmt.Errorf("deleting existing admin group %s: %w", g.ID, err) } + if len(oldMembers) > 0 { + b := store.NewRelationshipBuilder() + for _, om := range oldMembers { + b = b.Without(model.NewOrgResource(g.ID.String()), model.MemberRelation, model.NewUserSubject(om.Username)) + } + if err := s.store.Authz().WriteRelationships(ctx, b.Build()); err != nil { + return fmt.Errorf("removing authz relations for old admin group %s: %w", g.ID, err) + } + } } @@ for _, m := range adminGroup.Members { if _, err := s.store.Accounts().CreateMember(ctx, model.Member{ Username: m.Username, Email: m.Email, GroupID: group.ID, }); err != nil { return fmt.Errorf("adding admin member %s: %w", m.Username, err) } + updates := store.NewRelationshipBuilder(). + With(model.NewOrgResource(group.ID.String()), model.MemberRelation, model.NewUserSubject(m.Username)). + Build() + if err := s.store.Authz().WriteRelationships(ctx, updates); err != nil { + return fmt.Errorf("writing admin member authz relation %s: %w", m.Username, err) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/accounts.go` around lines 155 - 185, When initializing admin groups, ensure authz relationship bookkeeping is done: before calling s.store.Accounts().DeleteGroup for pre-existing admin groups, call s.store.Authz().WriteRelationships to remove the MemberRelation tuples for that group's members (matching what RemoveGroupMember does), and when adding bootstrapped members do not call s.store.Accounts().CreateMember directly — instead call the service-level CreateMember (the method on the service that wraps store account creation and writes the MemberRelation via s.store.Authz().WriteRelationships) or explicitly write the MemberRelation tuple after creating the member; both fixes should run in the same transaction already used by Initialize so the authz tuples stay consistent with the store.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/templates/admin-group-template.yml`:
- Around line 19-24: The template in deploy/templates/admin-group-template.yml
hardcodes a single admin entry under data.admin-group.yaml (members: with
${ADMIN_USERNAME}/${ADMIN_EMAIL}) and a fixed name: admin, preventing bootstrap
of multiple admins or different group names; change the template to accept a
multi-line parameter (e.g., ADMIN_MEMBERS) and inline it under members: (or
iterate over indexed vars like ADMIN_USERNAME_1/ADMIN_EMAIL_1) so multiple
entries can be rendered, and expose a GROUP_NAME parameter to replace the
hardcoded name: admin; update the template references to data.admin-group.yaml,
members, and name to use these parameters so the configmap can bootstrap
multiple admins and arbitrary group names.
In `@internal/api_server/server.go`:
- Around line 175-199: There are multiple redundant AccountsServicer instances
and Initialize is being called on the authz-wrapped service; create a single
unwrapped accounts service (use service.NewAccountsService or
service.NewAccountsServicer consistently) and assign it to an innerAccountsSvc
variable, reuse that single instance when constructing partnerSvc and
assessmentSvc (pass innerAccountsSvc into
NewPartnerService/NewAssessmentService) and only wrap with
NewAuthzAccountsService for the runtime accountsSvc variable; when loading the
admin group via service.ParseAdminGroupFile call
innerAccountsSvc.Initialize(ctx, *adminGroup) (not the authz-wrapped
accountsSvc) so boot initialization does not require an auth user.
In `@internal/handlers/v1alpha1/accounts.go`:
- Around line 54-63: Replace all instances where a *service.ErrForbidden branch
returns server.*403JSONResponse with Message: err.Error() by logging the error
(logger.Error(err).Log()) and returning a static, non-sensitive message (e.g.,
"forbidden") instead; update the 403 handling in ListGroups (the shown switch),
CreateGroup, GetGroup, UpdateGroup (both calls), DeleteGroup (both calls),
ListGroupMembers, CreateGroupMember, UpdateGroupMember (both calls), and
RemoveGroupMember so they mirror the default branch logging pattern and return a
constant string rather than the raw err.Error().
In `@internal/service/accounts_test.go`:
- Around line 479-599: Collapse the nine ParseAdminGroupFile specs into a
table-driven test: extract the os.CreateTemp/WriteString/Close/Remove
boilerplate into a helper (e.g. writeTempYAML) that returns the temp filename
and uses DeferCleanup to remove the file, then replace the repeated It blocks
with a single DescribeTable that calls service.ParseAdminGroupFile on
writeTempYAML(content) and asserts the error contains the expected substring;
use Entry rows for each case (empty name, whitespace name, empty members,
empty/whitespace username, empty/whitespace email) to keep tests concise and
reference ParseAdminGroupFile, writeTempYAML, DescribeTable, Entry, and
DeferCleanup when applying the change.
In `@internal/service/accounts.go`:
- Around line 42-72: In ParseAdminGroupFile validate member uniqueness and email
shape before returning: after trimming fields in ParseAdminGroupFile, check for
duplicate usernames (e.g., build a map[string]bool keyed by
ag.Members[i].Username and return a descriptive error if a duplicate is found)
and validate each ag.Members[i].Email with net/mail.ParseAddress (return an
error for invalid addresses); reference ParseAdminGroupFile, AdminGroup, and
ag.Members to locate where to add these checks so misconfigurations are caught
before CreateMember/Initialize DB calls.
- Around line 155-156: The current ListGroups call filters by both
ByName(adminGroup.Name) and ByCompany(adminGroup.Name) which ties admin groups
to company==name and prevents reconciling groups that share a Name but have a
different Company; update the call in Initialize (where
s.store.Accounts().ListGroups is invoked) to remove the
ByCompany(adminGroup.Name) filter so the lookup uses only ByName(...) and
ByKind("admin") unless the invariants require company==name — if that invariant
is intended instead add a one-line comment near this call (and near the admin
group creation) documenting that admin groups always set Company == Name so
future readers know this is deliberate.
In `@internal/service/authz_accounts.go`:
- Around line 21-23: The Initialize method on AuthzAccountsService bypasses
requireAdmin and must be documented and protected: add a clear doc comment on
AuthzAccountsService.Initialize stating it is bootstrap-only (called from
internal/api_server/server.go before HTTP serving), must not be called from
request handlers, and intentionally skips authz; additionally add a runtime
guard that panics or returns an error if Initialize is called when auth context
indicates an active request user (use auth.MustHaveUser or similar) OR
remove/move Initialize off the public AccountsServicer interface into a separate
bootstrap-only interface or concrete type so handlers cannot call it.
- Around line 99-108: Update AuthzAccountsService.requireAdmin to accept a
resource string and use the KindAdmin constant instead of the literal "admin":
call auth.MustHaveUser(ctx), use a.inner.GetIdentity as before, compare
identity.Kind to KindAdmin, and when not admin call NewErrForbidden(resource,
user.Username). Then update every call site (ListGroups, GetGroup, CreateGroup,
UpdateGroup, DeleteGroup, GetMember, ListGroupMembers, CreateMember,
UpdateGroupMember, RemoveGroupMember) to pass the appropriate resource label
("groups" for group ops, "members" or the correct member label for member ops).
Ensure imports/reference to KindAdmin (IdentityKind) are used from accounts.go.
In `@Makefile`:
- Line 223: The deploy-on-openshift and deploy-on-kind targets pass
MIGRATION_PLANNER_ADMIN_GROUP_FILE into service-template.yml but never create
the migration-planner-admin-group ConfigMap, so when
MIGRATION_PLANNER_ADMIN_GROUP_FILE is non-empty the pod crashes at startup;
update those Makefile targets to apply deploy/templates/admin-group-template.yml
(using the same templating/apply flow as deploy/e2e.mk) before applying
service-template.yml so the ConfigMap exists, ensuring you still pass
MIGRATION_PLANNER_ADMIN_GROUP_FILE into the template; alternatively, add a clear
note in the target help that callers must pre-create the
migration-planner-admin-group ConfigMap when MIGRATION_PLANNER_ADMIN_GROUP_FILE
is set.
---
Duplicate comments:
In `@internal/service/accounts.go`:
- Around line 155-185: When initializing admin groups, ensure authz relationship
bookkeeping is done: before calling s.store.Accounts().DeleteGroup for
pre-existing admin groups, call s.store.Authz().WriteRelationships to remove the
MemberRelation tuples for that group's members (matching what RemoveGroupMember
does), and when adding bootstrapped members do not call
s.store.Accounts().CreateMember directly — instead call the service-level
CreateMember (the method on the service that wraps store account creation and
writes the MemberRelation via s.store.Authz().WriteRelationships) or explicitly
write the MemberRelation tuple after creating the member; both fixes should run
in the same transaction already used by Initialize so the authz tuples stay
consistent with the store.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a4d0dd60-ba20-4442-8906-01a6521f92e2
📒 Files selected for processing (19)
Makefileapi/v1alpha1/openapi.yamlapi/v1alpha1/spec.gen.godeploy/e2e.mkdeploy/templates/admin-group-template.ymldeploy/templates/service-template.ymlgo.modinternal/api/client/client.gen.gointernal/api/server/server.gen.gointernal/api_server/server.gointernal/config/config.gointernal/handlers/v1alpha1/accounts.gointernal/handlers/v1alpha1/accounts_test.gointernal/handlers/v1alpha1/partner_test.gointernal/handlers/v1alpha1/source.gointernal/service/accounts.gointernal/service/accounts_test.gointernal/service/authz_accounts.gointernal/service/partner_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (2)
internal/service/accounts.go (1)
192-200:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInitialize bypasses member authz relationship writes.
At Line 193, members are inserted directly via
s.store.Accounts().CreateMember(...), but the authz relationship side effect used inCreateMember(Lines 293-299) is not executed. This can leave initialized admin members without expected authorization bindings.🔧 Suggested fix
for _, m := range adminGroup.Members { if _, err := s.store.Accounts().CreateMember(ctx, model.Member{ Username: m.Username, Email: m.Email, GroupID: group.ID, }); err != nil { return fmt.Errorf("adding admin member %s: %w", m.Username, err) } + + updates := store.NewRelationshipBuilder(). + With(model.NewOrgResource(group.ID.String()), model.MemberRelation, model.NewUserSubject(m.Username)). + Build() + if err := s.store.Authz().WriteRelationships(ctx, updates); err != nil { + return fmt.Errorf("adding admin member authz relation %s: %w", m.Username, err) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/accounts.go` around lines 192 - 200, The loop is calling the low-level s.store.Accounts().CreateMember(...) which inserts members but skips the authz-relationship side effect implemented in the CreateMember code path (the authz write at the CreateMember block around lines 293-299), leaving admin members without required authorization bindings; fix by invoking the higher-level service method that performs both account creation and authz wiring (i.e., call the service-level CreateMember wrapper instead of s.store.Accounts().CreateMember), or if no wrapper exists, after creating the member call the same authz relationship write used in CreateMember (the authz-relations writer function used in that block) to attach the member to the admin group so the authz binding is always created during initialization.internal/handlers/v1alpha1/accounts.go (1)
56-62:⚠️ Potential issue | 🟠 Major | ⚡ Quick win403 branches still leak
err.Error()and bypass logging.This was raised previously and remains unaddressed across all twelve
*service.ErrForbiddenbranches in this file (Lines 58, 92, 117, 153, 167, 192, 204, 227, 257, 272, 300, 333). Two concrete problems:
NewErrForbidden(resource, username)formats the requesting username (and resource label) into the message, soMessage: err.Error()exposes those identifiers in 403 bodies served to unauthenticated/unauthorized callers — this is a needless PII/auth-state leak.- All other branches in the same switches return static messages (e.g.,
"group not found","group already exists","empty body") and thedefaultbranch logs vialogger.Error(err).Log(). The 403 branches uniquely skip both, so authz denials are inconsistent with sibling responses and invisible in logs.Suggested shape (apply identically to every 403 branch):
🛡️ Proposed fix
- case *service.ErrForbidden: - return server.ListGroups403JSONResponse{Message: err.Error()}, nil + case *service.ErrForbidden: + logger.Error(err).Log() + return server.ListGroups403JSONResponse{Message: "forbidden"}, nilAlso applies to Lines 92, 117, 153, 167, 192, 204, 227, 257, 272, 300, 333.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/handlers/v1alpha1/accounts.go` around lines 56 - 62, The 403 branches currently return err.Error() (leaking usernames/resource identifiers) and skip logging; for every switch branch that checks for *service.ErrForbidden and returns a server.*403JSONResponse (e.g., ListGroups403JSONResponse and the eleven other 403 handlers), replace the dynamic Message: err.Error() with a static, generic message such as "forbidden" or "unauthorized" and add a logger.Error(err).Log() (or equivalent logging call) immediately before returning so the original error is recorded server-side; keep all other response shapes unchanged so behavior matches the sibling default error branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@internal/handlers/v1alpha1/accounts.go`:
- Around line 56-62: The 403 branches currently return err.Error() (leaking
usernames/resource identifiers) and skip logging; for every switch branch that
checks for *service.ErrForbidden and returns a server.*403JSONResponse (e.g.,
ListGroups403JSONResponse and the eleven other 403 handlers), replace the
dynamic Message: err.Error() with a static, generic message such as "forbidden"
or "unauthorized" and add a logger.Error(err).Log() (or equivalent logging call)
immediately before returning so the original error is recorded server-side; keep
all other response shapes unchanged so behavior matches the sibling default
error branches.
In `@internal/service/accounts.go`:
- Around line 192-200: The loop is calling the low-level
s.store.Accounts().CreateMember(...) which inserts members but skips the
authz-relationship side effect implemented in the CreateMember code path (the
authz write at the CreateMember block around lines 293-299), leaving admin
members without required authorization bindings; fix by invoking the
higher-level service method that performs both account creation and authz wiring
(i.e., call the service-level CreateMember wrapper instead of
s.store.Accounts().CreateMember), or if no wrapper exists, after creating the
member call the same authz relationship write used in CreateMember (the
authz-relations writer function used in that block) to attach the member to the
admin group so the authz binding is always created during initialization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d67ca813-025f-43d2-912a-6648dc78c70e
📒 Files selected for processing (19)
Makefileapi/v1alpha1/openapi.yamlapi/v1alpha1/spec.gen.godeploy/e2e.mkdeploy/templates/admin-group-template.ymldeploy/templates/service-template.ymlgo.modinternal/api/client/client.gen.gointernal/api/server/server.gen.gointernal/api_server/server.gointernal/config/config.gointernal/handlers/v1alpha1/accounts.gointernal/handlers/v1alpha1/accounts_test.gointernal/handlers/v1alpha1/partner_test.gointernal/handlers/v1alpha1/source.gointernal/service/accounts.gointernal/service/accounts_test.gointernal/service/authz_accounts.gointernal/service/partner_test.go
tupyy
force-pushed
the
auhtz-groups
branch
2 times, most recently
from
900971a to
d5ad34b
Compare
Jos-Jerus
reviewed
May 6, 2026
| ) | ||
|
|
||
| type AccountsServicer interface { | ||
| Initialize(ctx context.Context, adminGroup AdminGroup) error |
Collaborator
There was a problem hiding this comment.
do we really need Initialize on the AccountsServicer interface? its only called from server.go on boot where we already have innerAccountsSvc directly, so no reason to expose it on the interface where any handler could call it by mistake
| @@ -140,6 +152,9 @@ func (h *ServiceHandler) UpdateGroup(ctx context.Context, request server.UpdateG | |||
| existing, err := h.accountsSrv.GetGroup(ctx, request.Id) | |||
Collaborator
There was a problem hiding this comment.
on UpdateGroup the handler calls GetGroup then UpdateGroup on the authz wrapper so requireAdmin runs twice and we hit the DB for the same identity check on the same request. same thing on DeleteGroup. maybe worth having the handler use innerAccountsSvc for the initial read or combine it somehow
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nirarg The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This commit add auhtz to group endpoint. Also, it initiliaze the admin
group from an file:
```
name: platform-admins
members:
- username: alice
email: alice@redhat.com
- username: bob
email: bob@redhat.com
```
Signed-off-by: Cosmin Tupangiu <cosmin@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
internal/service/accounts.go (2)
192-199:⚠️ Potential issue | 🟠 MajorBootstrap member creation still skips the authz side effect.
Initializecreates admin members throughs.store.Accounts().CreateMember, but the normalCreateMemberpath also writes the org membership relationship. That leaves bootstrapped admins with different authz state than members created through the service.🔒 Suggested change
for _, m := range adminGroup.Members { if _, err := s.store.Accounts().CreateMember(ctx, model.Member{ Username: m.Username, Email: m.Email, GroupID: group.ID, }); err != nil { return fmt.Errorf("adding admin member %s: %w", m.Username, err) } + + updates := store.NewRelationshipBuilder(). + With(model.NewOrgResource(group.ID.String()), model.MemberRelation, model.NewUserSubject(m.Username)). + Build() + if err := s.store.Authz().WriteRelationships(ctx, updates); err != nil { + return fmt.Errorf("adding admin member authz relation %s: %w", m.Username, err) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/accounts.go` around lines 192 - 199, The bootstrap loop in Initialize uses the low-level s.store.Accounts().CreateMember which skips the authz/org-membership side effect; replace that call with the service-level member creation that performs authz (e.g., call the service's CreateMember method used elsewhere such as CreateMember on the Accounts service) so bootstrapped admins get the same org membership relationships, or alternatively after CreateMember explicitly create the org membership via the authz store (e.g., call the authz membership writer with the new member's ID and group/org ID) to mirror the normal CreateMember path.
75-78:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject duplicate usernames instead of silently dropping them.
continuemakes an invalid admin-group file look valid and changes the effective bootstrap config without any signal. For auth bootstrap, fail fast here with a descriptive error instead of keeping the first entry.♻️ Suggested change
- if _, exists := members[m.Username]; exists { - continue - } + if _, exists := members[m.Username]; exists { + return nil, fmt.Errorf("admin group file: duplicate username %q", m.Username) + } members[m.Username] = mAs per coding guidelines, “Input validation is mandatory.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/accounts.go` around lines 75 - 78, Currently duplicate usernames are silently ignored by the continue when populating the members map; change this to fail fast by returning a descriptive error (e.g. using fmt.Errorf) when members[m.Username] already exists. Edit the block that checks if _, exists := members[m.Username]; exists to return an error that includes m.Username and the source/context (the admin-group/bootstrap parser function) instead of continuing, so the caller can abort bootstrap/auth with a clear validation failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/templates/service-template.yml`:
- Around line 76-78: Update the MIGRATION_PLANNER_ADMIN_GROUP_FILE parameter
description to warn that the file path must be backed by the
migration-planner-admin-group ConfigMap; explicitly add a sentence like: "When
setting MIGRATION_PLANNER_ADMIN_GROUP_FILE ensure the
migration-planner-admin-group ConfigMap exists (see admin-group-template.yml) or
apply admin-group-template.yml before this template (as done in deploy/e2e.mk)."
Apply the same clarification to the other occurrences noted (lines near 641-642
and 689-692) so callers know to create/apply the ConfigMap before starting the
server.
In `@go.mod`:
- Line 43: Update the unmaintained module reference from "gopkg.in/yaml.v3" to
the maintained fork "go.yaml.in/yaml/v3" (use v3.0.4 or newer); change the
go.mod entry accordingly, replace all import paths in code from
"gopkg.in/yaml.v3" to "go.yaml.in/yaml/v3", then run "go get
go.yaml.in/yaml/v3@v3.0.4" (or desired version) and "go mod tidy" to update
go.sum and ensure builds succeed (check any usages in functions that call
yaml.Unmarshal / yaml.Marshal to confirm no API differences).
In `@internal/handlers/v1alpha1/accounts_test.go`:
- Around line 688-746: These nine nearly identical specs (authzSrv.ListGroups,
CreateGroup, GetGroup, UpdateGroup, DeleteGroup, ListGroupMembers,
CreateGroupMember, UpdateGroupMember, RemoveGroupMember) should be consolidated
into a single table-driven test using Ginkgo's DescribeTable: create a table
with entries that include the method name or a closure invoking the
corresponding authzSrv method and the expected response type (e.g.,
server.ListGroups403JSONResponse{}), then replace the repetitive It blocks with
a DescribeTable that calls each closure with nonAdminCtx() and asserts no error
and the 403 response type; this centralizes the 403 matrix and makes
adding/removing cases trivial.
---
Duplicate comments:
In `@internal/service/accounts.go`:
- Around line 192-199: The bootstrap loop in Initialize uses the low-level
s.store.Accounts().CreateMember which skips the authz/org-membership side
effect; replace that call with the service-level member creation that performs
authz (e.g., call the service's CreateMember method used elsewhere such as
CreateMember on the Accounts service) so bootstrapped admins get the same org
membership relationships, or alternatively after CreateMember explicitly create
the org membership via the authz store (e.g., call the authz membership writer
with the new member's ID and group/org ID) to mirror the normal CreateMember
path.
- Around line 75-78: Currently duplicate usernames are silently ignored by the
continue when populating the members map; change this to fail fast by returning
a descriptive error (e.g. using fmt.Errorf) when members[m.Username] already
exists. Edit the block that checks if _, exists := members[m.Username]; exists
to return an error that includes m.Username and the source/context (the
admin-group/bootstrap parser function) instead of continuing, so the caller can
abort bootstrap/auth with a clear validation failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cdbc233f-81ba-4f51-b288-3dc172027cae
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
Makefileapi/v1alpha1/openapi.yamlapi/v1alpha1/spec.gen.godeploy/e2e.mkdeploy/templates/admin-group-template.ymldeploy/templates/service-template.ymlgo.modinternal/api/client/client.gen.gointernal/api/server/server.gen.gointernal/api_server/server.gointernal/config/config.gointernal/handlers/v1alpha1/accounts.gointernal/handlers/v1alpha1/accounts_test.gointernal/handlers/v1alpha1/partner_test.gointernal/handlers/v1alpha1/source.gointernal/service/accounts.gointernal/service/accounts_test.gointernal/service/authz_accounts.gointernal/service/partner_test.go
Comment on lines
+76
to
+78
| - name: MIGRATION_PLANNER_ADMIN_GROUP_FILE | ||
| description: Path to YAML file defining the bootstrap admin group | ||
| value: "" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Document coordination requirement between admin group file path and ConfigMap.
The template allows MIGRATION_PLANNER_ADMIN_GROUP_FILE to be set while the migration-planner-admin-group ConfigMap is marked optional: true. If the parameter points to /etc/planner/admin-group.yaml but the ConfigMap doesn't exist, the server will start successfully but fail during initialization when attempting to read the configured file.
Consider adding a parameter description note: "When setting MIGRATION_PLANNER_ADMIN_GROUP_FILE, ensure the migration-planner-admin-group ConfigMap exists. See admin-group-template.yml."
Alternatively, as done in deploy/e2e.mk, callers should apply admin-group-template.yml before this template when using admin group initialization.
Also applies to: 641-642, 689-692
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/templates/service-template.yml` around lines 76 - 78, Update the
MIGRATION_PLANNER_ADMIN_GROUP_FILE parameter description to warn that the file
path must be backed by the migration-planner-admin-group ConfigMap; explicitly
add a sentence like: "When setting MIGRATION_PLANNER_ADMIN_GROUP_FILE ensure the
migration-planner-admin-group ConfigMap exists (see admin-group-template.yml) or
apply admin-group-template.yml before this template (as done in deploy/e2e.mk)."
Apply the same clarification to the other occurrences noted (lines near 641-642
and 689-692) so callers know to create/apply the ConfigMap before starting the
server.
| github.com/thoas/go-funk v0.9.3 | ||
| github.com/xuri/excelize/v2 v2.9.1 | ||
| go.uber.org/zap v1.27.0 | ||
| gopkg.in/yaml.v3 v3.0.1 |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for the latest version and any security advisories for gopkg.in/yaml.v3
# Check latest version on GitHub (yaml.v3 is from go-yaml/yaml)
echo "=== Checking latest yaml.v3 release ==="
gh api repos/go-yaml/yaml/releases/latest --jq '.tag_name'
# Check for security advisories
echo "=== Checking for security advisories ==="
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "gopkg.in/yaml.v3") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'Repository: kubev2v/migration-planner
Length of output: 568
🌐 Web query:
gopkg.in/yaml.v3 latest version 2025 2026
💡 Result:
As of May 20, 2026, the package gopkg.in/yaml.v3 is considered unmaintained and has been archived [1][2]. The final stable release for this specific import path remains v3.0.1 [3][4]. Due to its unmaintained status, the community and the YAML organization have transitioned to a new, actively supported fork [2]. Developers are advised to migrate to the new official package, which is available under the import path go.yaml.in/yaml/v3 [5][2]. As of June 2025, the latest version of this maintained package is v3.0.4 [5]. Additionally, a newer major version, go.yaml.in/yaml/v4, is also available for those seeking the latest features and ongoing support [5].
Citations:
- 1: https://github.com/go-yaml/yaml
- 2:
gopkg.in/yaml.v3is unmaintained, consider migrating tohttps://github.com/yaml/go-yamlgo-task/task#2171 - 3: https://pkg.go.dev/gopkg.in/yaml.v3?tab=versions
- 4: https://pkg.go.dev/gopkg.in/yaml.v3
- 5: https://pkg.go.dev/go.yaml.in/yaml/v3
Migrate from unmaintained gopkg.in/yaml.v3 to the supported fork go.yaml.in/yaml.
While gopkg.in/yaml.v3 v3.0.1 is secure against the known Denial of Service vulnerability (published 2022-05-20), the package itself is unmaintained and archived as of May 2026. The YAML community has transitioned to a maintained fork available at go.yaml.in/yaml/v3 with v3.0.4 as the latest stable release (June 2025), or go.yaml.in/yaml/v4 for newer features. Update the dependency to ensure ongoing security patching and community support for this configuration parsing library.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@go.mod` at line 43, Update the unmaintained module reference from
"gopkg.in/yaml.v3" to the maintained fork "go.yaml.in/yaml/v3" (use v3.0.4 or
newer); change the go.mod entry accordingly, replace all import paths in code
from "gopkg.in/yaml.v3" to "go.yaml.in/yaml/v3", then run "go get
go.yaml.in/yaml/v3@v3.0.4" (or desired version) and "go mod tidy" to update
go.sum and ensure builds succeed (check any usages in functions that call
yaml.Unmarshal / yaml.Marshal to confirm no API differences).
Comment on lines
+688
to
+746
| It("ListGroups returns 403 for non-admin", func() { | ||
| resp, err := authzSrv.ListGroups(nonAdminCtx(), server.ListGroupsRequestObject{}) | ||
| Expect(err).To(BeNil()) | ||
| Expect(resp).To(BeAssignableToTypeOf(server.ListGroups403JSONResponse{})) | ||
| }) | ||
|
|
||
| It("CreateGroup returns 403 for non-admin", func() { | ||
| body := v1alpha1.GroupCreate{Name: "X", Kind: v1alpha1.GroupCreateKindPartner, Icon: "i", Company: "C"} | ||
| resp, err := authzSrv.CreateGroup(nonAdminCtx(), server.CreateGroupRequestObject{Body: &body}) | ||
| Expect(err).To(BeNil()) | ||
| Expect(resp).To(BeAssignableToTypeOf(server.CreateGroup403JSONResponse{})) | ||
| }) | ||
|
|
||
| It("GetGroup returns 403 for non-admin", func() { | ||
| resp, err := authzSrv.GetGroup(nonAdminCtx(), server.GetGroupRequestObject{Id: uuid.New()}) | ||
| Expect(err).To(BeNil()) | ||
| Expect(resp).To(BeAssignableToTypeOf(server.GetGroup403JSONResponse{})) | ||
| }) | ||
|
|
||
| It("UpdateGroup returns 403 for non-admin", func() { | ||
| name := "X" | ||
| body := v1alpha1.GroupUpdate{Name: &name} | ||
| resp, err := authzSrv.UpdateGroup(nonAdminCtx(), server.UpdateGroupRequestObject{Id: uuid.New(), Body: &body}) | ||
| Expect(err).To(BeNil()) | ||
| Expect(resp).To(BeAssignableToTypeOf(server.UpdateGroup403JSONResponse{})) | ||
| }) | ||
|
|
||
| It("DeleteGroup returns 403 for non-admin", func() { | ||
| resp, err := authzSrv.DeleteGroup(nonAdminCtx(), server.DeleteGroupRequestObject{Id: uuid.New()}) | ||
| Expect(err).To(BeNil()) | ||
| Expect(resp).To(BeAssignableToTypeOf(server.DeleteGroup403JSONResponse{})) | ||
| }) | ||
|
|
||
| It("ListGroupMembers returns 403 for non-admin", func() { | ||
| resp, err := authzSrv.ListGroupMembers(nonAdminCtx(), server.ListGroupMembersRequestObject{Id: uuid.New()}) | ||
| Expect(err).To(BeNil()) | ||
| Expect(resp).To(BeAssignableToTypeOf(server.ListGroupMembers403JSONResponse{})) | ||
| }) | ||
|
|
||
| It("CreateGroupMember returns 403 for non-admin", func() { | ||
| body := v1alpha1.MemberCreate{Username: "u", Email: "u@x.com"} | ||
| resp, err := authzSrv.CreateGroupMember(nonAdminCtx(), server.CreateGroupMemberRequestObject{Id: uuid.New(), Body: &body}) | ||
| Expect(err).To(BeNil()) | ||
| Expect(resp).To(BeAssignableToTypeOf(server.CreateGroupMember403JSONResponse{})) | ||
| }) | ||
|
|
||
| It("UpdateGroupMember returns 403 for non-admin", func() { | ||
| email := openapi_types.Email("u@x.com") | ||
| body := v1alpha1.MemberUpdate{Email: &email} | ||
| resp, err := authzSrv.UpdateGroupMember(nonAdminCtx(), server.UpdateGroupMemberRequestObject{Id: uuid.New(), Username: "u", Body: &body}) | ||
| Expect(err).To(BeNil()) | ||
| Expect(resp).To(BeAssignableToTypeOf(server.UpdateGroupMember403JSONResponse{})) | ||
| }) | ||
|
|
||
| It("RemoveGroupMember returns 403 for non-admin", func() { | ||
| resp, err := authzSrv.RemoveGroupMember(nonAdminCtx(), server.RemoveGroupMemberRequestObject{Id: uuid.New(), Username: "u"}) | ||
| Expect(err).To(BeNil()) | ||
| Expect(resp).To(BeAssignableToTypeOf(server.RemoveGroupMember403JSONResponse{})) | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Use a table-driven spec for the 403 matrix.
These 9 specs are structurally identical and are good candidates for a single DescribeTable, which will make failures easier to maintain as handlers evolve.
♻️ Refactor sketch
- It("ListGroups returns 403 for non-admin", func() { ... })
- It("CreateGroup returns 403 for non-admin", func() { ... })
- ...
+ DescribeTable("returns 403 for non-admin",
+ func(call func(context.Context) (any, error), expected any) {
+ resp, err := call(nonAdminCtx())
+ Expect(err).To(BeNil())
+ Expect(resp).To(BeAssignableToTypeOf(expected))
+ },
+ Entry("ListGroups",
+ func(ctx context.Context) (any, error) {
+ return authzSrv.ListGroups(ctx, server.ListGroupsRequestObject{})
+ },
+ server.ListGroups403JSONResponse{},
+ ),
+ Entry("CreateGroup",
+ func(ctx context.Context) (any, error) {
+ body := v1alpha1.GroupCreate{Name: "X", Kind: v1alpha1.GroupCreateKindPartner, Icon: "i", Company: "C"}
+ return authzSrv.CreateGroup(ctx, server.CreateGroupRequestObject{Body: &body})
+ },
+ server.CreateGroup403JSONResponse{},
+ ),
+ // ...remaining endpoints...
+ )As per coding guidelines: "Readability: Use table-driven tests for multiple cases."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/handlers/v1alpha1/accounts_test.go` around lines 688 - 746, These
nine nearly identical specs (authzSrv.ListGroups, CreateGroup, GetGroup,
UpdateGroup, DeleteGroup, ListGroupMembers, CreateGroupMember,
UpdateGroupMember, RemoveGroupMember) should be consolidated into a single
table-driven test using Ginkgo's DescribeTable: create a table with entries that
include the method name or a closure invoking the corresponding authzSrv method
and the expected response type (e.g., server.ListGroups403JSONResponse{}), then
replace the repetitive It blocks with a DescribeTable that calls each closure
with nonAdminCtx() and asserts no error and the 403 response type; this
centralizes the 403 matrix and makes adding/removing cases trivial.
Collaborator
|
/lgtm |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds authorization to the group endpoint. Also, it initializes the admin group from a file:
Signed-off-by: Cosmin Tupangiu cosmin@redhat.com
Summary by CodeRabbit
New Features
API Changes
Deployment / Chores
Tests